[BR-1794]: Add rate limit retry mechanism with exponential backoff#1834
[BR-1794]: Add rate limit retry mechanism with exponential backoff#1834
Conversation
Implement retry utility to handle 429 rate limit errors with configurable backoff strategy, extract rate limit info from response headers, and apply it to public shared folder content requests to improve reliability under rate limiting conditions.
Deploying drive-web with
|
| Latest commit: |
34d5f3b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82de3e28.drive-web.pages.dev |
| Branch Preview URL: | https://feature-retry-with-backoff.drive-web.pages.dev |
CandelR
left a comment
There was a problem hiding this comment.
Is this accompanied by some kind of visual information for the user? I don't see the point of this PR from the user's point of view, as they will be waiting at least 60 seconds without knowing what is happening, which is an enormous amount of time.
| return typeof error === 'object' && error !== null; | ||
| } | ||
|
|
||
| function isRateLimitError(error: unknown): boolean { |
There was a problem hiding this comment.
Use arrow functions to be consistent with rest of the codebase
| return ( | ||
| error.status === 429 || | ||
| error.statusCode === 429 || | ||
| error.response?.status === 429 || |
There was a problem hiding this comment.
If I remember correctly, there are status codes, use them or add this one if it is not there
| return undefined; | ||
| } | ||
|
|
||
| const resetValueMs = Number.parseInt(headers['x-ratelimit-reset'], 10); |
There was a problem hiding this comment.
the unit that return the server is in miliseconds?
| } | ||
| } | ||
|
|
||
| throw new Error('Maximum retries exceeded'); |
There was a problem hiding this comment.
Is this error reachable? Check it with tests
| maxRetries: 5, | ||
| maxDelay: 60000, | ||
| onRetry: (attempt, delay) => { | ||
| console.log( |
There was a problem hiding this comment.
maybe better use console warn if want to log the retry for when user records the console and send to us
| if (!isErrorWithStatus(error) || !error.headers) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
this is already checked in isRateLimitError, I think that never will reach this code
| }; | ||
| } | ||
|
|
||
| export async function retryWithBackoff<T>(fn: () => Promise<T>, options: RetryOptions = {}): Promise<T> { |
There was a problem hiding this comment.
add jsdoc to this exported function
- Remove maxDelay option and related fallback logic - Simplify rate limit detection to only check status codes (429) - Update rate limit header from 'x-ratelimit-reset' to 'x-internxt-ratelimit-reset' - Remove limit and remaining fields from RateLimitInfo interface - Add user-facing toast notification on first retry attempt - Convert functions to arrow functions for consistency - Add HTTP_CODES.TOO_MANY_REQUESTS constant - Add JSDoc documentation to retryWithBackoff function - Add internationalization support for retry notification - Update tests to reflect new behavior and validate toast integration - Fix loop condition to prevent unreachable code
52a8dc6 to
cd0f3ad
Compare
|
|
||
| if (!hasShownRateLimitToast) { | ||
| hasShownRateLimitToast = true; | ||
| notificationsService.show({ |
There was a problem hiding this comment.
I would not mix UI related things with pure logic like this function. Return something that allows the function consumer to determine if showing this notification or not proceeds
56215d9 to
6b7760c
Compare
| "link-updated": "Linkeinstellungen aktualisiert", | ||
| "link-deleted": "Link gelöscht", | ||
| "links-deleted": "Link(s) gelöscht" | ||
| "links-deleted": "Link(s) gelöscht", |
There was a problem hiding this comment.
Seems that here is missing error-deleting-links translation, can you add it? :)
| hasShownRateLimitNotification = true; | ||
| notificationsService.show({ | ||
| text: t('shared-links.toast.rate-limit-retry'), | ||
| type: ToastType.Warning, | ||
| duration: Infinity, | ||
| }); |
There was a problem hiding this comment.
Move this out of the service, return the error, and handle it in the manager component of the UI to decide what to display. This way, we try to keep it clean. Even if there is a function with the notificationService in this service, we will try not to mix it up any further :)
|



Description
Implement retry utility to handle 429 rate limit errors with configurable backoff strategy, extract rate limit info
from response headers, and apply it to public shared folder content requests to improve reliability under rate
limiting conditions.
Related Issues
Related Pull Requests
Checklist
Testing Process
Additional Notes